Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Events for exams IDA #268

Merged
merged 30 commits into from
Oct 2, 2023
Merged

Conversation

ilee2u
Copy link
Contributor

@ilee2u ilee2u commented Sep 19, 2023

Description: We are developing a new backend for exams called edx-exams. This backend will be utilized by instructor dashboards to interact with student exam attempts, and will store data regarding those attempts.

We are currently working to implement the downstream effects triggered whenever an exam attempt is submitted or reviewed. For example, when an exam attempt is submitted, we will want to make sure edx-platform knows to mark the exam subsection.

We have added several event definitions based on different actions that can be performed on exam attempts:

  • EXAM_ATTEMPT_SUBMITTED
  • EXAM_ATTEMPT_REJECTED
  • EXAM_ATTEMPT_VERIFIED
  • EXAM_ATTEMPT_ERRORED
  • EXAM_ATTEMPT_RESET

These events will be consumed by various services in edx-platform in order to perform tasks such as creating grade overrides, invalidating certificates, changing credit requirement statuses, and more.

ISSUE: MST-2083 (Internal)

Dependencies: None.

Merge deadline: List merge deadline (if any)

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

  1. Open page A
  2. Do thing B
  3. Expect C to happen
  4. If D happened instead - check failed.

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@ilee2u ilee2u force-pushed the exams-ida-hackathon branch from 423b066 to 6424a40 Compare September 21, 2023 15:40
@ilee2u ilee2u changed the title Events for exams IDA (draft/experimental), might not be PR'd this way. Events for exams IDA Sep 21, 2023
credit_requirement_status (str): status to change the student's CreditRequirement to
"""

user_id = attr.ib(type=int)
Copy link
Contributor

@zacharis278 zacharis278 Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed @MichaelRoytman's PR uses the UserData object here. Do you think we should use that? The LMS can look it up but it might be nice to match how other events are sending user id / email / etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the parameters of the downstream functions, it looks like most of them take user_id as a parameter anyways, so I think user_id is the way to go here.

Also I believe Michael's PR just has placeholder data for now.

Copy link

@MichaelRoytman MichaelRoytman Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to use the UserData. It seems to be more common to use that than a user_id. But I do see uses of both, so there's precedence to use both.

I don't think we have to model our events off the way they're being consumed now. Ideally, they shouldn't be so context aware. I think, for me, it's less, "the Instructor Service currently only expects a user_id so that's what we should put in the event" and more "how can I best and most generically represent the concept of a user in my event"?

Is sending PII when we don't need to a consideration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a good chance future downstream logic would use the pii. This probably gets hooked up to name affirmation at some point.

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment on how we send user info. Otherwise this looks good to me

Copy link

@MichaelRoytman MichaelRoytman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@ilee2u ilee2u marked this pull request as ready for review September 21, 2023 18:44
@ilee2u ilee2u requested a review from a team as a code owner September 21, 2023 18:44

# .. event_type: org.openedx.learning.exam.attempt.submitted.v1
# .. event_name: EXAM_ATTEMPT_SUBMITTED
# .. event_description: Emitted when an exam attempt is submitted by a learner.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some high-level questions:

  1. Does the platform have other types of "exams" that would not trigger these events? If so, how do we differentiate? Note that an event is defined by its event trigger + event data.
  2. For the exams covered by these events, are there multiple exam types? Would it be useful to know that as part of the event data?
  3. If there are different exam types, do some of these events only apply to certain types of exam types? If so, would that be useful to note?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. kind of. The existing exam system (edx-proctoring) does not trigger these events, the service that emits these is intended to replacement. There are no other types of exams outside that.
  2. Good point. There are multiple exam types and edx-platform is mostly unaware of this since all the state/ui flows are manage by the exams/proctoring service. That said, I think there is value including this. Consumers may likely care if an exam is proctored or not. We might have actually missed a need for this. I think Credits depend on if an exam is proctored.
  3. Yup some of these events are impossible depending on the exam type. A timed exam cannot be rejected since there is no video review to reject for example. We should add those details to the event descriptions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there non-timed exams, and are those included in this as well? Or are these just timed-exams, proctored and not-proctored? Just curious.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I guess this doesn't include 'all' exams just 'special exams' which have a different UX and workflow. Right now, all special exam types are a subset of timed. This wouldn't include normal sections instructors are using as or are graded as exams however. We could possibly reintroduce the term 'special exam' for these events which could clear this up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think of this more as an API discussion around exams and special-exams, who do you think should be a part of this discussion? Maybe https://openedx.atlassian.net/wiki/spaces/COMM/pages/3554214289/Arch+Coordination+-+How+to+announce+and+tune-in will help you get the right discussion going?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ I think this is a good idea to have before we merge. I could see people getting confused on two points

  1. these events only pertain to 'special' exams
  2. these events are not published by the existing exams system

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the following changes to address this:

  • Changed the data name to "SpecialExamAttemptData"
  • Added a comment about how events using this data are only emitted for special exams from the newer exams backend, edx-exams

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better! Thank you folks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to push hard on this, because it's not my domain of expertise, but the naming difference between EXAM_ATTEMPT_SUBMITTED and SpecialExamAttemptData just catches my attention. The obvious other option is to rename to SPECIAL_EXAM_ATTEMPT_SUBMITTED , and I guess the question I have is around the pros/cons of going with the more specific name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 🤔 It's a good question, I'd almost lean towards not using 'special' in the name at all and just commenting to the meaning? Otherwise, we need to consistently use the term across Open edX. I don't think 'special exam' has any additional meaning to anyone working outside this domain unless it's described. There was a actually discussion a while back that we stop using this term because it's just confusing, trying to dig that up.

I'm not sure how to best describe the fact that this intends to include all content that is treated with an exam user experience, but not content that might be titled (on the section or grading policy) by course teams as 'exam'. The latter is ultimately no different from other content, the platform doesn't do anything functionally different. (hence not special)

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there folks. I left a few minor comments. Thanks!

@ilee2u ilee2u force-pushed the exams-ida-hackathon branch from 37dd373 to 7e5cabd Compare September 28, 2023 19:28
CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor doc fix before we merge.

@ilee2u
Copy link
Contributor Author

ilee2u commented Sep 29, 2023

Hi, we've been informed that v8.6.0, which this PR is based off, is having issues, so please do not merge this until we find a solution/workaround for those issues.

CHANGELOG.rst Outdated Show resolved Hide resolved
@mariajgrimaldi mariajgrimaldi added the blocked by other work PR cannot be finished until other work is complete label Sep 29, 2023
@ilee2u ilee2u force-pushed the exams-ida-hackathon branch 2 times, most recently from a360649 to ca3ff5e Compare September 29, 2023 18:23
Decision
********

* Since `edx-exams` is independent, we will want to prevent circular dependencies between `edx-platform` and `edx-exams`.
Copy link
Contributor

@zacharis278 zacharis278 Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a decision that impacts this events library and is better explained in detail by the ADR you already linked above.


* Since `edx-exams` is independent, we will want to prevent circular dependencies between `edx-platform` and `edx-exams`.
* We have defined the events that we plan to define in `this ADR <https://github.com/edx/edx-exams/blob/main/docs/decisions/0004-downstream-effect-events.rst>`_ in `edx-exams` to use the event bus, as the new exams backend is independent in this ADR
* We plan for these events to be produced in `edx-exams`, and consumed in various `edx-platform` services (e.g. certificates, credit, instructor, grades).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can flip our focus here to be less about edx-exams and more how these events impact edx-platform. Maybe

  • exam events will be consumed by edx-platform to handle all behavior as it pertains to the state of an exam subsection.
  • the edx-exams service will produce these events
  • there is no plan to have edx-proctoring produce these events


**Draft** 2023-09-29

Context
Copy link
Contributor

@zacharis278 zacharis278 Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the context section might be a good place to explain the meaning of exam / special exam.

Rough points to hit

  • course subsections with an exam_type have additional logic that governs completion, grading, credit requirements, and more based on the type of exam.
  • these sections are sometimes referred to as special exams
  • course subsections without an exam type configured may still have a grading policy named 'Exam'. This type of content does not have the exam user experience and is not governed by any exam specific logic.

@ilee2u ilee2u force-pushed the exams-ida-hackathon branch from 273f27a to 9e4744c Compare October 2, 2023 14:28
@mariajgrimaldi mariajgrimaldi merged commit b36ea94 into openedx:main Oct 2, 2023
6 checks passed
@mariajgrimaldi mariajgrimaldi removed the blocked by other work PR cannot be finished until other work is complete label Oct 2, 2023
@mariajgrimaldi
Copy link
Member

Hi @ilee2u! This PR introduced an ADR with the status draft. Do you think we can change the ADR's status to accepted by now?

@ilee2u
Copy link
Contributor Author

ilee2u commented Oct 16, 2024

Hi @ilee2u! This PR introduced an ADR with the status draft. Do you think we can change the ADR's status to accepted by now?

Hi again, yes we should be able to! Thanks :)

@ilee2u ilee2u mentioned this pull request Oct 16, 2024
11 tasks
@ilee2u
Copy link
Contributor Author

ilee2u commented Oct 16, 2024

Hi @ilee2u! This PR introduced an ADR with the status draft. Do you think we can change the ADR's status to accepted by now?

Hi again, yes we should be able to! Thanks :)

Just made a PR: #410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants